-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implements filter mechanism and improves filter at backend side for AAS Repository #516
base: main
Are you sure you want to change the base?
Implements filter mechanism and improves filter at backend side for AAS Repository #516
Conversation
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
* | ||
* @author danish | ||
*/ | ||
public interface BaSyxCrudRepository<T> extends CrudRepository<T, String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes:
- consider using the Spring interface QuerydsIPredicateExecutor.
- I think this generic interface may not be necessary. I would consider adding a class to the core backend of the repositories/services/registries themselves instead.
- consider renaming this interface (or interfaces, if you think (2) is valid) to
backend
; the keywordrepository
is used with different semantics in the context of this project. We decided to usebackend
for the backend management of the Spring side - which happens to be called repositories. Maybe we can come up with a better naming convention later 😅
* | ||
* @author danish | ||
*/ | ||
public class MongoDBCrudRepository<T> extends SimpleMongoRepository<T, String> implements BaSyxCrudRepository<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not necessary with QuerydslPredicateExecutor. See querydsl-mongodb.
@@ -77,7 +78,7 @@ public String getBaseUrl() { | |||
} | |||
|
|||
@Override | |||
public CursorResult<List<AssetAdministrationShell>> getAllAas(PaginationInfo pInfo) { | |||
public CursorResult<List<AssetAdministrationShell>> getAllAas(PaginationInfo pInfo, Filter filter) { | |||
String encodedCursor = pInfo.getCursor() == null ? null : Base64UrlEncoder.encode(pInfo.getCursor()); | |||
return repoApi.getAllAssetAdministrationShells(null, null, pInfo.getLimit(), encodedCursor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's currently not matching the capabilities of the http controller. The two nulls could be retrieved from the filter object
@Test | ||
@Ignore | ||
public void retrieveAllAasWithSpecificAssetIdAndIdShortFiltering() throws Exception { | ||
//TODO: Clients doesn't support filtering, remove this when client has the functionality |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the internal api supports it. See remark in ConnectedAasRepository.
@@ -75,7 +77,7 @@ public void removeAasFromRepo() throws FileNotFoundException, IOException { | |||
TestAuthorizedConnectedAasService.configureSecurityContext(TestAuthorizedConnectedAasService.getTokenProvider()); | |||
|
|||
AasRepository repo = appContext.getBean(AasRepository.class); | |||
repo.getAllAas(PaginationInfo.NO_LIMIT).getResult().stream().map(s -> s.getId()).forEach(repo::deleteAas); | |||
repo.getAllAas(PaginationInfo.NO_LIMIT, new AasFilter()).getResult().stream().map(s -> s.getId()).forEach(repo::deleteAas); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AasFilter.NO_FILTER or simply passing null?
* | ||
* @author danish | ||
*/ | ||
public class TestAasMongoDBFilterResolution { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider reusing the TestInMemoryAasFilterResolution by extracting a testsuite. The test coverage is considerably different for component with the same function.
@@ -50,7 +51,7 @@ public interface AasRepository { | |||
* | |||
* @return a list of all found Asset Administration Shells | |||
*/ | |||
public CursorResult<List<AssetAdministrationShell>> getAllAas(PaginationInfo pInfo); | |||
public CursorResult<List<AssetAdministrationShell>> getAllAas(PaginationInfo pInfo, Filter filter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider overloading this method. There are lot of instances of an empty filter being passed and this measure would reduce quite a bit the amount changes of this PR. Also make sure to document the @param
return resolveCursor(pRequest, data, Function.identity()); | ||
} | ||
|
||
public static <T> String resolveCursor(PaginationInfo pRequest, List<T> data, Function<T, String> idResolver) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider adding these two abstract methods to PagginationSupport, since it already serves as a util class. Also consider leaving this refactoring to another PR. It's not necessarily related to the scope of the current's PR and increases quite a bit the amount of changes (when considering, for example, the changes in aasdiscoveryservice)
@@ -56,7 +56,7 @@ | |||
*/ | |||
public class CrudAasDiscovery implements AasDiscoveryService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes here are not necessarily related to the PR. See remark in common.pagination
@@ -34,7 +34,7 @@ public class CrudAasDiscoveryTest { | |||
|
|||
@Test | |||
public void getConfiguredAasDiscoveryName(){ | |||
AasDiscoveryService service = new SimpleAasDiscoveryFactory(null, CONFIGURED_AAS_DISCOVERY_NAME).create(); | |||
AasDiscoveryService service = new SimpleAasDiscoveryFactory(() -> null, CONFIGURED_AAS_DISCOVERY_NAME).create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to pr
@@ -0,0 +1,9 @@ | |||
package org.eclipse.digitaltwin.basyx.core; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
License header
Description of Changes
The current implementation of CrudRepository does filtering InMemory only. However, it should be done on the backend side only based on specific implementation.
This PR adds support for filtering (Pagination/Metamodel) at the backend only. This PR adds some useful, common, and necessary classes as generic so that the Filtering can be applied easily to other repositories and services.
In this PR, the Filter and filtering at the backend feature is defined as a generic way to be used by other modules but the implementation has been done only for the AAS Repository. The reason is that the size of the PR would be way big if all the modules were considered. After this PR other modules implementation will follow.
Affected methods:
getAll*() -> Pagination
Related Issue
#437, #532